Skip to content

[WIP] Migrate command #7693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from
Draft

[WIP] Migrate command #7693

wants to merge 38 commits into from

Conversation

zth
Copy link
Collaborator

@zth zth commented Jul 21, 2025

  • Tracks deprecated usages in the cmt
  • Uses that info to produce/apply migrations to a single file, according to a user-defined template

Instructions for adding more migrations to the stdlib

  • Add new @deprecated with a migration prop to anything in the stdlib
  • Run make lib so it's built into the stdlib files

In tests/tools_tests:

  • Run yarn rescript legacy clean to ensure you get the new changes
  • Add sample usages to a StdlibMigration_ModuleName.res file
  • IF NEEDED (as in if the migration will not be able to compile after being done, because of type mismatches that cannot be fixed automatically or similar), add your sample usage to a StdlibMigrationNoCompile_ModuleName.res file instead. This will not get compiled for testing (but it will get migrated)
  • Run make (still inside of tests/tools_tests) to run run the migrations and builds. This should get your test output

Repeat the process above until done.

TODO

  • Add errors on invalid structure of the @deprecated attribute
  • Error if the wrong type of migration template is provided (ident for function, etc)
  • Do deep replace of argument mapping, so nested things like someThing(~someArg=[Some(%insert.unlabelledArg(1))]) works
  • Explore supporting mapping unlabelled arg 0 (using the _ mechanism)
  • Explore type checking the templates
  • Explore how tests for migrations can easily be written

@zth zth marked this pull request as draft July 21, 2025 19:34
@zth zth force-pushed the tools-migrate branch from b485278 to a3d443f Compare July 22, 2025 16:55
Copy link

pkg-pr-new bot commented Jul 22, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7693

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7693

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7693

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7693

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7693

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7693

commit: cbb9407

@cristianoc cristianoc requested a review from Copilot July 23, 2025 08:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a new migrate command for ReScript that tracks deprecated usage in compiled TypeScript (.cmt) files and applies user-defined migration templates to automatically update code. The implementation includes comprehensive stdlib migrations for transitioning from Js.Array2.* functions to modern Array.* equivalents.

  • Adds a complete migration framework with AST transformation capabilities for function calls, pipes, and references
  • Integrates deprecated usage tracking into the compiler's type checking and CMT generation pipeline
  • Provides extensive test coverage with stdlib array function migrations and validation of both compilable and non-compilable migration scenarios

Reviewed Changes

Copilot reviewed 37 out of 38 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/src/tools.ml Core migration command implementation with AST mapper and template application logic
tests/tools_tests/test.sh Test framework integration for migration command testing
tests/tools_tests/src/migrate/* Test files demonstrating various migration scenarios and expected outputs
runtime/Js_array2.res Adds @deprecated annotations with migration templates for all Js.Array2 functions
compiler/ml/* Compiler integration for tracking deprecated usage in CMT files
runtime/Stdlib_Array.resi Documentation additions for Array copyWithin functions
Comments suppressed due to low confidence (3)

tools/src/tools.ml:1316

  • [nitpick] The variable name labelled_args_that_will_be_mapped is verbose and could be shortened to mapped_labelled_args or labelled_to_map for better readability.
      let labelled_args_that_will_be_mapped = ref [] in

tools/src/tools.ml:1317

  • [nitpick] The variable name unlabelled_positions_that_will_be_mapped is excessively long and could be shortened to mapped_positions or positions_to_map.
      let unlabelled_positions_that_will_be_mapped = ref [] in

tests/tools_tests/src/migrate/StdlibMigration_Array.res:150

  • The test demonstrates parameter reordering in reduce functions but lacks verification that the migration correctly handles the parameter position changes from (fn, initial) to (initial, fn).
let reduce1 = [1, 2, 3]->Js.Array2.reduce((acc, x) => acc + x, 0)

expr =
(fun mapper exp ->
match exp with
| {pexp_desc = Pexp_ident {loc}}
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern matching for pexp_desc = Pexp_ident {loc} could be more specific by including the identifier in the pattern to make the intent clearer and avoid potential issues if the location check fails.

Suggested change
| {pexp_desc = Pexp_ident {loc}}
| {pexp_desc = Pexp_ident {loc; txt}}

Copilot uses AI. Check for mistakes.

};
}
| _ ->
(* TODO: More elaborate warnings etc *)
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TODO comment lacks specificity about what kind of warnings should be implemented. Consider adding more detail about the expected behavior when migration templates are invalid.

Suggested change
(* TODO: More elaborate warnings etc *)
(* TODO: Implement detailed warnings for invalid migration templates.
Consider logging a warning with the location and details of the invalid template.
Decide whether to fallback to a default behavior or propagate the error. *)

Copilot uses AI. Check for mistakes.

Comment on lines +1659 to +1745
(* TODO: More elaborate warnings etc *)
(* Invalid config. *)
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate TODO comment - consider consolidating error handling logic into a shared function rather than repeating the same TODO in multiple places.

Suggested change
(* TODO: More elaborate warnings etc *)
(* Invalid config. *)
handle_warning "Invalid configuration encountered during transformation.";

Copilot uses AI. Check for mistakes.

Comment on lines +1719 to +1805
(* TODO: More elaborate warnings etc *)
(* Invalid config. *)
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third occurrence of the same TODO comment. This indicates a need for a centralized error handling approach for invalid migration configurations.

Suggested change
(* TODO: More elaborate warnings etc *)
(* Invalid config. *)
handle_invalid_migration_config ~context:"Invalid migration template in deprecated function call";

Copilot uses AI. Check for mistakes.

Comment on lines +108 to +110
(* TODO: Validate and error if expected shape mismatches *)
match reason with
| Some reason -> Some (reason, migration_template)
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation of migration template structure could lead to runtime errors or incorrect migrations. Input validation should be implemented to ensure the migration template has the expected format.

Suggested change
(* TODO: Validate and error if expected shape mismatches *)
match reason with
| Some reason -> Some (reason, migration_template)
let validate_migration_template template =
match template.pexp_desc with
| Pexp_constant (Pconst_string (_, _)) -> Some template
| _ -> None
in
let validated_template = Option.bind migration_template validate_migration_template in
match reason with
| Some reason -> Some (reason, validated_template)

Copilot uses AI. Check for mistakes.

@zth zth force-pushed the tools-migrate branch from 9818a10 to 0081583 Compare July 24, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants